From 15e4cf374350b81db5cb82f618673aeeedf55d0a Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Mon, 21 Jul 2014 19:06:52 -0700 Subject: [PATCH] Simplify the ProcessBuilder struct This changes many bounds to ToCStr to stay in line with the since-introduced Command structure. The builder remains separate of command to have control over executing and Show. Path-related methods have been removed and env-initialization/management are left to Command, ProcessBuilder only keeps track of the delta. --- src/cargo/core/package.rs | 6 +- src/cargo/ops/cargo_rustc/mod.rs | 131 ++++++++++++++---------------- src/cargo/util/process_builder.rs | 122 ++++++++-------------------- 3 files changed, 99 insertions(+), 160 deletions(-) diff --git a/src/cargo/core/package.rs b/src/cargo/core/package.rs index e70f86384..96175b9a1 100644 --- a/src/cargo/core/package.rs +++ b/src/cargo/core/package.rs @@ -1,4 +1,3 @@ -use std::cmp; use std::fmt::{Show,Formatter}; use std::fmt; use std::slice; @@ -122,8 +121,9 @@ impl Package { let mut sources = self.get_source_ids(); // Sort the sources just to make sure we have a consistent fingerprint. sources.sort_by(|a, b| { - cmp::lexical_ordering(a.kind.cmp(&b.kind), - a.location.to_string().cmp(&b.location.to_string())) + let a = (&a.kind, a.location.to_string()); + let b = (&b.kind, b.location.to_string()); + a.cmp(&b) }); let sources = sources.iter().map(|source_id| { source_id.load(config) diff --git a/src/cargo/ops/cargo_rustc/mod.rs b/src/cargo/ops/cargo_rustc/mod.rs index df4115539..bef79e548 100644 --- a/src/cargo/ops/cargo_rustc/mod.rs +++ b/src/cargo/ops/cargo_rustc/mod.rs @@ -16,8 +16,6 @@ mod job; mod job_queue; mod layout; -type Args = Vec; - // This is a temporary assert that ensures the consistency of the arguments // given the current limitations of Cargo. The long term fix is to have each // Target know the absolute path to the build location. @@ -152,10 +150,8 @@ fn compile_custom(pkg: &Package, cmd: &str, } let mut p = util::process(cmd.next().unwrap()) .cwd(pkg.get_root()) - .env("OUT_DIR", Some(output.as_str() - .expect("non-UTF8 dest path"))) - .env("DEPS_DIR", Some(output.as_str() - .expect("non-UTF8 dest path"))) + .env("OUT_DIR", Some(&output)) + .env("DEPS_DIR", Some(&output)) .env("TARGET", cx.config.target()); for arg in cmd { p = p.arg(arg); @@ -205,50 +201,41 @@ fn rustc(package: &Package, target: &Target, fn prepare_rustc(package: &Package, target: &Target, crate_types: Vec<&str>, cx: &Context, req: PlatformRequirement) -> Vec { let root = package.get_root(); - let mut target_args = Vec::new(); - build_base_args(&mut target_args, target, crate_types.as_slice(), cx, false); - build_deps_args(&mut target_args, package, cx, false); - - let mut plugin_args = Vec::new(); - build_base_args(&mut plugin_args, target, crate_types.as_slice(), cx, true); - build_deps_args(&mut plugin_args, package, cx, true); let base = util::process("rustc").cwd(root.clone()); + let base = build_base_args(base, target, crate_types.as_slice()); + + let target = build_plugin_args(base.clone(), cx, false); + let plugin = build_plugin_args(base, cx, true); + let target = build_deps_args(target, package, cx, false); + let plugin = build_deps_args(plugin, package, cx, true); match req { - Target => vec![base.args(target_args.as_slice())], - Plugin => vec![base.args(plugin_args.as_slice())], - PluginAndTarget if cx.config.target().is_none() => - vec![base.args(target_args.as_slice())], - PluginAndTarget => - vec![base.clone().args(target_args.as_slice()), - base.args(plugin_args.as_slice())], + Target => vec![target], + Plugin => vec![plugin], + PluginAndTarget if cx.config.target().is_none() => vec![target], + PluginAndTarget => vec![target, plugin], } } -fn build_base_args(into: &mut Args, +fn build_base_args(mut cmd: ProcessBuilder, target: &Target, - crate_types: &[&str], - cx: &Context, - plugin: bool) { + crate_types: &[&str]) -> ProcessBuilder { let metadata = target.get_metadata(); // TODO: Handle errors in converting paths into args - into.push(target.get_src_path().display().to_string()); + cmd = cmd.arg(target.get_src_path()); - into.push("--crate-name".to_string()); - into.push(target.get_name().to_string()); + cmd = cmd.arg("--crate-name").arg(target.get_name()); for crate_type in crate_types.iter() { - into.push("--crate-type".to_string()); - into.push(crate_type.to_string()); + cmd = cmd.arg("--crate-type").arg(*crate_type); } let profile = target.get_profile(); if profile.get_opt_level() != 0 { - into.push("--opt-level".to_string()); - into.push(profile.get_opt_level().to_string()); + cmd = cmd.arg("--opt-level").arg(profile.get_opt_level().to_string()); } // Right now -g is a little buggy, so we're not passing -g just yet @@ -257,87 +244,91 @@ fn build_base_args(into: &mut Args, // } if !profile.get_debug() { - into.push("--cfg".to_string()); - into.push("ndebug".to_string()); + cmd = cmd.args(["--cfg", "ndebug"]); } if profile.is_test() { - into.push("--test".to_string()); + cmd = cmd.arg("--test"); } match metadata { Some(m) => { - into.push("-C".to_string()); - into.push(format!("metadata={}", m.metadata)); - - into.push("-C".to_string()); - into.push(format!("extra-filename={}", m.extra_filename)); + cmd = cmd.arg("-C").arg(format!("metadata={}", m.metadata)); + cmd = cmd.arg("-C").arg(format!("extra-filename={}", m.extra_filename)); } None => {} } + return cmd; +} - into.push("--out-dir".to_string()); - into.push(cx.layout(plugin).root().display().to_string()); + +fn build_plugin_args(mut cmd: ProcessBuilder, cx: &Context, + plugin: bool) -> ProcessBuilder { + cmd = cmd.arg("--out-dir"); + cmd = cmd.arg(cx.layout(plugin).root()); if !plugin { - fn opt(into: &mut Vec, key: &str, prefix: &str, - val: Option<&str>) { + fn opt(cmd: ProcessBuilder, key: &str, prefix: &str, + val: Option<&str>) -> ProcessBuilder { match val { Some(val) => { - into.push(key.to_string()); - into.push(format!("{}{}", prefix, val)); + cmd.arg(key) + .arg(format!("{}{}", prefix, val)) } - None => {} + None => cmd } } - opt(into, "--target", "", cx.config.target()); - opt(into, "-C", "ar=", cx.config.ar()); - opt(into, "-C", "linker=", cx.config.linker()); + cmd = opt(cmd, "--target", "", cx.config.target()); + cmd = opt(cmd, "-C", "ar=", cx.config.ar()); + cmd = opt(cmd, "-C", "linker=", cx.config.linker()); } + + return cmd; } -fn build_deps_args(dst: &mut Args, package: &Package, cx: &Context, - plugin: bool) { +fn build_deps_args(mut cmd: ProcessBuilder, package: &Package, cx: &Context, + plugin: bool) -> ProcessBuilder { let layout = cx.layout(plugin); - dst.push("-L".to_string()); - dst.push(layout.root().display().to_string()); - dst.push("-L".to_string()); - dst.push(layout.deps().display().to_string()); + cmd = cmd.arg("-L").arg(layout.root()); + cmd = cmd.arg("-L").arg(layout.deps()); // Traverse the entire dependency graph looking for -L paths to pass for // native dependencies. - push_native_dirs(dst, &layout, package, cx, &mut HashSet::new()); + cmd = push_native_dirs(cmd, &layout, package, cx, &mut HashSet::new()); for &(_, target) in cx.dep_targets(package).iter() { let layout = cx.layout(target.get_profile().is_plugin()); for filename in cx.target_filenames(target).iter() { - dst.push("--extern".to_string()); - dst.push(format!("{}={}/{}", - target.get_name(), - layout.deps().display(), - filename)); + let mut v = Vec::new(); + v.push_all(target.get_name().as_bytes()); + v.push(b'='); + v.push_all(layout.deps().as_vec()); + v.push(b'/'); + v.push_all(filename.as_bytes()); + cmd = cmd.arg("--extern").arg(v.as_slice()); } } - fn push_native_dirs(dst: &mut Args, layout: &layout::LayoutProxy, + return cmd; + + fn push_native_dirs(mut cmd: ProcessBuilder, layout: &layout::LayoutProxy, pkg: &Package, cx: &Context, - visited: &mut HashSet) { - if !visited.insert(pkg.get_package_id().clone()) { return } + visited: &mut HashSet) -> ProcessBuilder { + if !visited.insert(pkg.get_package_id().clone()) { return cmd } if pkg.get_manifest().get_build().len() > 0 { - dst.push("-L".to_string()); - dst.push(layout.native(pkg).display().to_string()); + cmd = cmd.arg("-L").arg(layout.native(pkg)); } match cx.resolve.deps(pkg.get_package_id()) { Some(mut pkgids) => { - for dep_id in pkgids { + pkgids.fold(cmd, |cmd, dep_id| { let dep = cx.get_package(dep_id); - push_native_dirs(dst, layout, dep, cx, visited); - } + push_native_dirs(cmd, layout, dep, cx, visited) + }) } - None => {} + None => cmd } } } diff --git a/src/cargo/util/process_builder.rs b/src/cargo/util/process_builder.rs index 5b6e81b10..fff9cd63b 100644 --- a/src/cargo/util/process_builder.rs +++ b/src/cargo/util/process_builder.rs @@ -10,76 +10,53 @@ use util::{ProcessError, process_error}; #[deriving(Clone,PartialEq)] pub struct ProcessBuilder { program: CString, - args: Vec, - path: Vec, - env: HashMap, - cwd: Path + args: Vec, + env: HashMap>, + cwd: Path, } impl Show for ProcessBuilder { fn fmt(&self, f: &mut Formatter) -> fmt::Result { - try!(write!(f, "`{}", self.program.as_str().unwrap_or(""))); + try!(write!(f, "`{}", String::from_utf8_lossy(self.program.as_bytes_no_nul()))); - if self.args.len() > 0 { - try!(write!(f, " {}", self.args.connect(" "))); + for arg in self.args.iter() { + try!(write!(f, " {}", String::from_utf8_lossy(arg.as_bytes_no_nul()))); } write!(f, "`") } } -// TODO: Upstream a Windows/Posix branch to Rust proper -#[cfg(unix)] -static PATH_SEP : &'static str = ":"; -#[cfg(windows)] -static PATH_SEP : &'static str = ";"; - impl ProcessBuilder { - pub fn arg(mut self, arg: T) -> ProcessBuilder { - self.args.push(arg.as_slice().to_string()); + pub fn arg(mut self, arg: T) -> ProcessBuilder { + self.args.push(arg.to_c_str()); self } - pub fn args(mut self, arguments: &[T]) -> ProcessBuilder { - self.args = arguments.iter().map(|a| a.as_slice().to_string()).collect(); + pub fn args(mut self, arguments: &[T]) -> ProcessBuilder { + self.args.extend(arguments.iter().map(|t| t.to_c_str())); self } - pub fn get_args(&self) -> &[String] { + pub fn get_args(&self) -> &[CString] { self.args.as_slice() } - pub fn extra_path(mut self, path: Path) -> ProcessBuilder { - // For now, just convert to a string, but we should do something better - self.path.unshift(path.display().to_string()); - self - } - pub fn cwd(mut self, path: Path) -> ProcessBuilder { self.cwd = path; self } - pub fn env(mut self, key: &str, val: Option<&str>) -> ProcessBuilder { - match val { - Some(v) => { - self.env.insert(key.to_string(), v.to_string()); - }, - None => { - self.env.remove(&key.to_string()); - } - } - + pub fn env(mut self, key: &str, val: Option) -> ProcessBuilder { + self.env.insert(key.to_string(), val.map(|t| t.to_c_str())); self } // TODO: should InheritFd be hardcoded? pub fn exec(&self) -> Result<(), ProcessError> { let mut command = self.build_command(); - command - .env_set_all(self.build_env().as_slice()) - .stdout(InheritFd(1)) - .stderr(InheritFd(2)); + command.stdout(InheritFd(1)) + .stderr(InheritFd(2)); let msg = || format!("Could not execute process `{}`", self.debug_string()); @@ -95,8 +72,7 @@ impl ProcessBuilder { } pub fn exec_with_output(&self) -> Result { - let mut command = self.build_command(); - command.env_set_all(self.build_env().as_slice()); + let command = self.build_command(); let msg = || format!("Could not execute process `{}`", self.debug_string()); @@ -115,65 +91,37 @@ impl ProcessBuilder { pub fn build_command(&self) -> Command { let mut command = Command::new(self.program.as_bytes_no_nul()); - command.args(self.args.as_slice()).cwd(&self.cwd); - command - } - - fn debug_string(&self) -> String { - let program = self.program.as_str().unwrap_or(""); - if self.args.len() == 0 { - program.to_string() - } else { - format!("{} {}", program, self.args.connect(" ")) + command.cwd(&self.cwd); + for arg in self.args.iter() { + command.arg(arg.as_bytes_no_nul()); } - } - - fn build_env(&self) -> Vec<(String, String)> { - let mut ret = Vec::new(); - - for (key, val) in self.env.iter() { - // Skip path - if key.as_slice() != "PATH" { - ret.push((key.clone(), val.clone())); + for (k, v) in self.env.iter() { + let k = k.as_slice(); + match *v { + Some(ref v) => { command.env(k, v.as_bytes_no_nul()); } + None => { command.env_remove(k); } } } - - match self.build_path() { - Some(path) => ret.push(("PATH".to_string(), path)), - _ => () - } - - ret.to_vec() + command } - fn build_path(&self) -> Option { - let path = self.path.connect(PATH_SEP); - - match self.env.find_equiv(&("PATH")) { - Some(existing) => { - if self.path.is_empty() { - Some(existing.clone()) - } else { - Some(format!("{}{}{}", existing, PATH_SEP, path)) - } - }, - None => { - if self.path.is_empty() { - None - } else { - Some(path) - } - } + fn debug_string(&self) -> String { + let program = String::from_utf8_lossy(self.program.as_bytes_no_nul()); + let mut program = program.into_string(); + for arg in self.args.iter() { + program.push_char(' '); + let s = String::from_utf8_lossy(arg.as_bytes_no_nul()); + program.push_str(s.as_slice()); } + program } } pub fn process(cmd: T) -> ProcessBuilder { ProcessBuilder { program: cmd.to_c_str(), - args: vec!(), - path: vec!(), + args: Vec::new(), cwd: os::getcwd(), - env: os::env().move_iter().collect() + env: HashMap::new(), } } -- 2.30.2